Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hardened compiler flags by default #12895

Merged
merged 563 commits into from
Aug 29, 2016
Merged

Hardened compiler flags by default #12895

merged 563 commits into from
Aug 29, 2016

Conversation

globin
Copy link
Member

@globin globin commented Feb 9, 2016

This adds some compiler/linker flags to harden our packages via a stdenv adapter:

The following parameters are now available:

  • hardeningDisable
    To disable specific hardening flags
  • hardeningEnable
    To enable specific hardening flags

Only the cc-wrapper supports this right now, but these may be reused by other wrappers, builders or setup hooks.

cc-wrapper supports the following flags:

  • fortify
  • stackprotector
  • pie (disabled by default)
  • pic
  • strictoverflow
  • format
  • relro
  • bindnow

Information from the debian wiki:

Stack protector is a mainline GCC feature, which adds safety checks against stack overwrites. This renders many potential code injection attacks into aborting situations. In the best case this turns code injection vulnerabilities into denial of service or into non-issues (depending on the application). http://en.wikipedia.org/wiki/Stack-smashing_protection
Currently this uses -fstack-protector-all instead of -fstack-protector-strong because the bootstrapping compiler is too old.` /cc @edolstra

Fortify During code generation the compiler knows a great deal of information about buffer sizes (where possible), and attempts to replace insecure unlimited length buffer function calls with length-limited ones. This is especially useful for old, crufty code. Additionally, format strings in writable memory that contain '%n' are blocked. If an application depends on such a format string, it will need to be worked around.

Format
If -Wformat is specified, also warn about uses of format functions that represent possible security problems. At present, this warns about calls to printf and scanf functions where the format string is not a string literal and there are no format arguments, as in printf (foo);. This may be a security hole if the format string came from untrusted input and contains %n.

Position Independent Executable (pie) are needed to take advantage of Address Space Layout Randomization, supported by some kernel versions. While ASLR can already be enforced for data areas in the stack and heap (brk and mmap), the code areas must be compiled as position-independent. Shared libraries already do this (-fPIC), so they gain ASLR automatically, but binary .text regions need to be build PIE to gain ASLR. When this happens, ROP attacks are much harder since there are no static locations to bounce off of during a memory corruption attack.

relro
During program load, several ELF memory sections need to be written to by the linker, but can be turned read-only before turning over control to the program. This prevents some GOT (and .dtors) overwrite attacks, but at least the part of the GOT used by the dynamic linker (.got.plt) is still vulnerable.

bindnow
During program load, all dynamic symbols are resolved, allowing for the complete GOT to be marked read-only (due to -z relro above). This prevents GOT overwrite attacks. For very large application, this can incur some performance loss during initial load while symbols are resolved, but this shouldn't be an issue for daemons.

State in other Distributions
Nearly all major distributions are setting these flags by default (except for pie), debian started working on this ~10 years ago, see links at the end.

Most packages build (https://hydra.nixos.org/jobset/nixpkgs/pr-12895#tabs-evaluations), mostly needs further testing at runtime.

I'd be happy to hear more comments, ideas, concerns!

Links
https://wiki.debian.org/Hardening
https://wiki.ubuntu.com/Security/Features
https://wiki.gentoo.org/wiki/Project:Hardened (and https://wiki.gentoo.org/wiki/Hardened/FAQ)
https://wiki.archlinux.org/index.php/DeveloperWiki:Security

(/cc @copumpkin @fpletz)

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @rbvermaa and @MarcWeber to be potential reviewers

@copumpkin
Copy link
Member

First off, awesome effort, thank you 👍 👍 👍 👍 🍻

Second, I know you say that it hasn't been tested on Darwin, but I'm wondering if you've tested things using the clang stdenv even on Linux. If so, I have reasonable confidence that all issues Darwin might encounter would be resolved ahead of time.

Edit: is PIE off by default due to performance concerns? I believe on Mac OS, it's on by default for x86_64 due to it being cheap.

@copumpkin copumpkin mentioned this pull request Feb 9, 2016
17 tasks
@heydojo
Copy link
Contributor

heydojo commented Feb 9, 2016

Is there a global switch to turn this all off. Or should I be looking at clang? (For prototyping and boards like pie zero where I want speed, don't want the penalties this creates.)

I would be interested in benchmarks to see what kind of a difference these changes make.

@globin
Copy link
Member Author

globin commented Feb 9, 2016

Yes hardening_all = false;

@heydojo
Copy link
Contributor

heydojo commented Feb 9, 2016

@globin cool tnx!

@copumpkin
Copy link
Member

@globin your evaluation error with hardening.foo is due to your hardening arguments making their way down to the underlying derivation. Nix assumes that all parameters to derivation (the builtin underlying mkDerivation) should be stringified and put into the environment of the underlying script. That's not as important in our case, but is still happening with your code. When you had hardening_foo, that was a boolean so could easily be stringified. When you wrote hardening.foo, that turned into a dict-valued attribute called hardening to derivation, and when Nix tries to stringify hardening, it doesn't know what to do.

@vcunat vcunat added 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 1.severity: security Issues which raise a security issue, or PRs that fix one labels Feb 9, 2016
@vcunat
Copy link
Member

vcunat commented Feb 9, 2016

Currently this uses -fstack-protector-all instead of -fstack-protector-strong because the bootstrapping compiler is too old.

There's no use for hardening during bootstrapping so you can selectively disable it in there. BTW, I didn't know gcc-4.8.3 is considered so old.

This includes a gcc update to gcc5 as we were checking all builds anyway and fixed up some stuff on the way.

We've had an unresolved discussion around gcc5 switch due to the question whether to use the new C++ ABI by default (already). (Full conformance to C++11 vs. inability to link with stuff compiled by older gcc versions.) #8729

@copumpkin
Copy link
Member

I didn't know gcc-4.8.3 is considered so old.

Hey, it was released over a year and a half ago. That's ages in computer time 😄

@globin
Copy link
Member Author

globin commented Feb 9, 2016

Ok sounds like I'll take the gcc bump out again.

Regarding GCC 4.8:

Since GCC 4.9, -fstack-protector-strong, an improved version of -fstack-protector is introduced, which covers all the more paranoid conditions that might lead to a stack overflow but not trade performance like -fstack-protector-all, thus it becomes default.

@copumpkin
Copy link
Member

Is moving to 4.9 less controversial than 5?
On Tue, Feb 9, 2016 at 18:26 Robin Gloster [email protected] wrote:

Ok sounds like I'll take the gcc bump out again.

Regarding GCC 4.8:

Since GCC 4.9, -fstack-protector-strong, an improved version of
-fstack-protector is introduced, which covers all the more paranoid
conditions that might lead to a stack overflow but not trade performance
like -fstack-protector-all, thus it becomes default.


Reply to this email directly or view it on GitHub
#12895 (comment).

@globin
Copy link
Member Author

globin commented Feb 9, 2016

gcc49 is the default for the system, only the pre-built bootstrap gcc is at 4.8

@globin
Copy link
Member Author

globin commented Feb 11, 2016

@copumpkin PIE is off because it breaks nearly everything (see the Ubuntu link where they have turned it on), I reckon you're thinking of PIC.

@domenkozar
Copy link
Member

FYI, travis is saying:

error: anonymous function at /home/travis/build/NixOS/nixpkgs/pkgs/development/compilers/webdsl/default.nix:1:1 called without required argument ‘strategoPackages’, at /home/travis/build/NixOS/nixpkgs/lib/customisation.nix:56:12

@globin globin mentioned this pull request Feb 21, 2016
@jagajaga
Copy link
Member

👍

@fpletz fpletz changed the title [WIP/RFC] Hardened compiler flags by default Hardened compiler flags by default Aug 23, 2016
@mrobbetts
Copy link
Contributor

mrobbetts commented Aug 24, 2016

I just updated my hardened-stdenv branch to get hold of today's commit(s) (I was up to date with it yesterday) and the huge rebuild is failing at the pcre package with:

----->8------->8----

FAIL: pcre_jit_test
===================

Running JIT regression tests
  target CPU of SLJIT compiler: x86 64bit (little endian + unaligned)
  in  8 bit mode with UTF-8  enabled and ucp enabled:

8 bit: JIT compiler does not support: AbC
8 bit: Test should match: [1] 'AbC' @ 'AbAbC'
FAIL pcre_jit_test (exit status: 139)

FAIL: RunTest
=============


PCRE C library tests using test data from ./testdata
PCRE version 8.38 2015-11-23

---- Testing 8-bit library ----

Test 1: Main functionality (Compatible with Perl >= 5.10)
  OK
  OK with study
  OK with JIT study
Test 2: API, errors, internals, and non-Perl stuff (not UTF-8)
  OK
  OK with study
  OK with JIT study
Cannot test locale-specific features - none of the 'fr_FR', 'fr' or
'french' locales exist, or the "locale" command is not available
to check for them.

Test 4: UTF-8 support (Compatible with Perl >= 5.10)
  OK
  OK with study
  OK with JIT study
Test 5: API, internals, and non-Perl stuff for UTF-8 support
  OK
  OK with study
  OK with JIT study
Test 6: Unicode property support (Compatible with Perl >= 5.10)
  OK
  OK with study
  OK with JIT study
Test 7: API, internals, and non-Perl stuff for Unicode property support
  OK
  OK with study
  OK with JIT study
Test 8: DFA matching main functionality
  OK
  OK with study
Test 9: DFA matching with UTF-8
  OK
  OK with study
Test 10: DFA matching with Unicode properties
  OK
  OK with study
Test 11: Internal offsets and code size tests
  OK
  OK with study
Test 12: JIT-specific features (when JIT is available)
--- ./testdata/testoutput12 2015-09-02 08:53:57.000000000 +0000
+++ testtry 2016-08-24 03:59:09.652704792 +0000
@@ -9,7 +9,7 @@
 Need char = 'c'
 Subject length lower bound = 3
 No starting char list
-JIT study was successful
+JIT study was not successful

 /(?(?C1)(?=a)a)/S+I
 Capturing subpattern count = 0
@@ -46,7 +46,7 @@
 Need char = 'c'
 Subject length lower bound = 3
 No starting char list
-JIT study was successful
+JIT study was not successful
 Compiled pattern written to testsavedregex
 Study data written to testsavedregex

@@ -66,45 +66,45 @@

 /(?(R)a*(?1)|((?R))b)/S+
     aaaabcde
-Error -27 (JIT stack limit reached)
+Error -26 (nested recursion at the same subject position)

 /-- Test various compile modes --/ 

 /abcd/S++
     abcd
- 0: abcd (JIT)
+ 0: abcd
     xyz  
-No match (JIT)
+No match

 /abcd/S+
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
-No match (JIT)
+No match

 /abcd/S++
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
-No match (JIT)
+No match

 /abcd/S++1
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
 Partial match: ab
     ab\P\P
 Partial match: ab
     xyz
-No match (JIT)
+No match
     xyz\P
 No match

@@ -112,7 +112,7 @@
     abcd
  0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
 Partial match: ab
     xyz
@@ -120,13 +120,13 @@

 /abcd/S++3
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
 Partial match: ab
     xyz
-No match (JIT)
+No match

 /abcd/S++4
     abcd
@@ -134,39 +134,39 @@
     ab\P
 Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
 No match

 /abcd/S++5
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
 Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
-No match (JIT)
+No match

 /abcd/S++6
     abcd
  0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
 No match

 /abcd/S++7
     abcd
- 0: abcd (JIT)
+ 0: abcd
     ab\P
-Partial match: ab (JIT)
+Partial match: ab
     ab\P\P
-Partial match: ab (JIT)
+Partial match: ab
     xyz
-No match (JIT)
+No match

 /abcd/S++2I 
 Capturing subpattern count = 0
@@ -175,15 +175,15 @@
 Need char = 'd'
 Subject length lower bound = 4
 No starting char list
-JIT study was successful
+JIT study was not successful

 /(*NO_START_OPT)a(*:m)b/KS++
     a
-No match, mark = m (JIT)
+No match, mark = m

 /^12345678abcd/mS++
     12345678abcd
- 0: 12345678abcd (JIT)
+ 0: 12345678abcd

 /-- Test pattern compilation --/ 

FAIL RunTest (exit status: 1)

============================================================================
Testsuite summary for PCRE 8.38
============================================================================
# TOTAL: 3
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
============================================================================
Makefile:2612: recipe for target 'test-suite.log' failed
make[3]: *** [test-suite.log] Error 1
make[3]: Leaving directory '/tmp/nix-build-pcre-8.38.drv-0/pcre-8.38'
Makefile:2718: recipe for target 'check-TESTS' failed
make[2]: *** [check-TESTS] Error 2
make[2]: Leaving directory '/tmp/nix-build-pcre-8.38.drv-0/pcre-8.38'
Makefile:2957: recipe for target 'check-am' failed
make[1]: *** [check-am] Error 2
make[1]: Leaving directory '/tmp/nix-build-pcre-8.38.drv-0/pcre-8.38'
Makefile:2960: recipe for target 'check' failed
make: *** [check] Error 2
gcc -DHAVE_CONFIG_H -I. -I../..  -DLOCALEDIR=\"/nix/store/xgrdpjxaavnrqqdbxkdpa2rlbbqw0dnv-xz-5.2.2/share/locale\" -I../../src/common -I../../src/liblzma/api -I../../lib  -pthread -fvisibility=hidden -Wall -Wextra -Wvla -Wformat=2 -Winit-self -Wmissing-include-dirs -Wstrict-aliasing -Wfloat-equal -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wlogical-op -Waggregate-return -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wredundant-decls -g -O2 -c -o xz-tuklib_open_stdxxx.o `test -f '../common/tuklib_open_stdxxx.c' || echo './'`../common/tuklib_open_stdxxx.c
builder for ‘/nix/store/xs41msw0gz3wb62k6px78nx4mn317g0k-pcre-8.38.drv’ failed with exit code 2
cannot build derivation ‘/nix/store/84hrf7w9ga2jhk1rb2pmld4qcysjzdcq-gnugrep-2.25.drv’: 1 dependencies couldn't be built
cannot build derivation ‘/nix/store/53198hfgxgzik3ji7rfc9jx8p7w7b1np-stdenv.drv’: 1 dependencies couldn't be built
cannot build derivation ‘/nix/store/g37vl831cmj1qwzcz8ay1zym2ww934ay-nix-1.11.2.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/g37vl831cmj1qwzcz8ay1zym2ww934ay-nix-1.11.2.drv’ failed

I also get the following the my dmesg (I'm using a grsecurity kernel):

[Tue Aug 23 20:53:48 2016] grsec: denied resource overstep by requesting 4096 for RLIMIT_NOFILE against limit 4096 for /tmp/nix-build-patch-2.7.5.drv-0/patch-2.7.5/conftest[conftest:18810] uid/euid:30010/30010 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:18809] uid/euid:30010/30010 gid/egid:30000/30000
[Tue Aug 23 20:53:48 2016] grsec: denied resource overstep by requesting 4294967295 for RLIMIT_NOFILE against limit 4096 for /tmp/nix-build-patch-2.7.5.drv-0/patch-2.7.5/conftest[conftest:18810] uid/euid:30010/30010 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:18809] uid/euid:30010/30010 gid/egid:30000/30000
[Tue Aug 23 20:53:48 2016] grsec: denied resource overstep by requesting 4294967295 for RLIMIT_NOFILE against limit 4096 for /tmp/nix-build-patch-2.7.5.drv-0/patch-2.7.5/conftest[conftest:19209] uid/euid:30010/30010 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:19207] uid/euid:30010/30010 gid/egid:30000/30000
[Tue Aug 23 20:53:48 2016] grsec: denied resource overstep by requesting 4096 for RLIMIT_NOFILE against limit 4096 for /tmp/nix-build-patch-2.7.5.drv-0/patch-2.7.5/conftest[conftest:19209] uid/euid:30010/30010 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:19207] uid/euid:30010/30010 gid/egid:30000/30000
[Tue Aug 23 20:53:48 2016] grsec: denied resource overstep by requesting 4096 for RLIMIT_NOFILE against limit 4096 for /tmp/nix-build-patch-2.7.5.drv-0/patch-2.7.5/conftest[conftest:19589] uid/euid:30010/30010 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:19588] uid/euid:30010/30010 gid/egid:30000/30000
[Tue Aug 23 20:53:56 2016] grsec: denied RWX mmap of <anonymous mapping> by /tmp/nix-build-pcre-8.38.drv-0/pcre-8.38/.libs/lt-pcre_jit_test[lt-pcre_jit_tes:28278] uid/euid:30003/30003 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:28273] uid/euid:30003/30003 gid/egid:30000/30000
[Tue Aug 23 20:53:56 2016] lt-pcre_jit_tes[28278]: segfault at 28 ip 000003effca90148 sp 000003fe76b7a760 error 4 in libc-2.23.so[3effca1b000+198000]
[Tue Aug 23 20:53:56 2016] grsec: Segmentation fault occurred at 0000000000000028 in /tmp/nix-build-pcre-8.38.drv-0/pcre-8.38/.libs/lt-pcre_jit_test[lt-pcre_jit_tes:28278] uid/euid:30003/30003 gid/egid:30000/30000, parent /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/bash[bash:28273] uid/euid:30003/30003 gid/egid:30000/30000
[Tue Aug 23 20:53:56 2016] audit: type=1701 audit(1472010785.889:123): auid=4294967295 uid=30003 gid=30000 ses=4294967295 pid=28278 comm="lt-pcre_jit_tes" exe="/tmp/nix-build-pcre-8.38.drv-0/pcre-8.38/.libs/lt-pcre_jit_test" sig=11

This make sense to anyone here?

@globin
Copy link
Member Author

globin commented Aug 24, 2016

@mrobbetts there was a bug in yesterdays commits.

@mrobbetts
Copy link
Contributor

@globin ah, fabulous. Rebuilding now :)

@mrobbetts
Copy link
Contributor

Is #17999 another such case? It is failing for me now.

@obadz
Copy link
Contributor

obadz commented Aug 27, 2016

@edolstra, are you OK for us to merge staging if http://hydra.nixos.org/build/39310332#tabs-constituents looks good?

@obadz
Copy link
Contributor

obadz commented Aug 28, 2016

The tests looked pretty good but the Golang ecosystem was broken by the binutils version bump in 0c12ae5. Was fixed by 6eb4014.

If there are no objections (cc @edolstra) AND the latest eval looks clean (http://hydra.nixos.org/build/39336299#tabs-constituents), I will merge staging tomorrow.

@domenkozar
Copy link
Member

There are still 65 "newly failing jobs" compared to trunk-combined: http://hydra.nixos.org/eval/1289558?compare=trunk-combined

@globin
Copy link
Member Author

globin commented Aug 28, 2016

But 97 newly succeeding Jobs 🙈
I guess we should merge nonetheless as it's getting hard to fix stuff without creating conflicts on master regularly.. I'm constantly fixing up new stuff and real failures disregarding dependencies are 3 or 4 packages that look unrelated to hardening at least

@obadz
Copy link
Contributor

obadz commented Aug 28, 2016

Agreed, unless @edolstra (or someone else) has concerns, let's go ahead and merge and deal with the minor remaining issues (which likely won't require a mass rebuild) in master.

@obadz
Copy link
Contributor

obadz commented Aug 28, 2016

@domenkozar, are you concerned enough that you're saying we shouldn't merge?

@domenkozar
Copy link
Member

If you're willing to help fix those later (which I'm sure you do), let's merge if there are no objections from others.

@edolstra
Copy link
Member

Looks great to me. Thanks for all the work on this!

@obadz obadz merged commit c0fa26e into master Aug 29, 2016
@globin globin deleted the hardened-stdenv branch August 29, 2016 15:49
@offlinehacker
Copy link
Contributor

Great work guys, thanks :)

@cleverca22
Copy link
Contributor

cleverca22 commented Sep 4, 2016

https://gist.github.com/cleverca22/b1300b91ea6bf8951256c60acf2844ee

__memcpy_chk appears to be missing from the libc.a in glibc.static master, but it is present on an older nixpkgs from 20f009d

edit:
ah, that function just moved to ${stdenv.cc.cc.out}/lib/libssp.a for some strange reason, -lssp solves the problem

@vcunat
Copy link
Member

vcunat commented Sep 11, 2016

Hmm, it seems we still have glibc without stack protector: #1.

@fpletz
Copy link
Member

fpletz commented Sep 11, 2016

Thanks for pointing that out. We had to disable stackprotector for the glibc build but didn't investigate further. There are even more hardening options available in the configure script for glibc. I'll check what we can enable. Unfortunately, this will be another mass rebuild that probably won't make it into 16.09.

@andrewrk
Copy link
Member

Great work on this issue. Is there a way so that we can have these hardening flags applied to nixpkgs, but not always forcing gcc users to use the flags? See #18995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: mass-rebuild This PR causes a large number of packages to rebuild 1.severity: security Issues which raise a security issue, or PRs that fix one 8.has: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.